Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#2480] Add NLDS paragraph components #1215

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

jiromaykin
Copy link
Contributor

@jiromaykin jiromaykin commented May 21, 2024

issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/2480

paragraph modifiers that Utrecht doesn't have are: --compact, --small, --muted, --centered
This means we will have to make "--oip" modifiers with our own designtokens, so that municipalities that do not wish to use our design-tokens can use either their own or add custom-styling themselves.

HTML example of the proper way of working:
This is what it looked/looks like in the past/legacy code:
<p class="p p--small">…..

But now this should become:
<p class="utrecht-paragraph utrecht-paragraph--oip utrecht-paragraph--oip-small">.....

We will not yet be implementing this for ALL <p> tags because we have them in our own components that do not exist in NLDS at all. (Like the paragraphs inside the 'Mijn profiel' tabled sections for example).

Also: in future I would love to completely remove all "legacy styling" that's meant to keep things backwards compatible for now. Plus it seems we have unused code, like location-card-list which might be safe to delete but not sure. So 'clean-up' will need to become a separate issue.

This PR relies on changes in our NLDS token package PR nr.9 so to view and check to see if all styles look the same, do an NPM ci --legacy-peer-deps install and rebuild.

Note to self: the two unique classes utrecht-paragraph--oip-title-text and utrecht-paragraph--oip-summary are not in design tokens becasue they should probably be defined by their parent component (like "readmore" component.

@jiromaykin jiromaykin force-pushed the feature/2480-add-NLDS-paragraphs branch 2 times, most recently from 4f0d754 to fffc00d Compare May 23, 2024 13:22
@codecov-commenter
Copy link

codecov-commenter commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.22%. Comparing base (95f207f) to head (674573f).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1215   +/-   ##
========================================
  Coverage    95.22%   95.22%           
========================================
  Files          968      968           
  Lines        35249    35249           
========================================
  Hits         33566    33566           
  Misses        1683     1683           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jiromaykin jiromaykin force-pushed the feature/2480-add-NLDS-paragraphs branch 5 times, most recently from 702c0f2 to 29aaf04 Compare May 28, 2024 12:17
@jiromaykin jiromaykin marked this pull request as ready for review May 28, 2024 12:17
@jiromaykin jiromaykin requested a review from pi-sigma May 28, 2024 14:13
@jiromaykin
Copy link
Contributor Author

@Bartvaderkin and @pi-sigma : So here is another fun big changes NLDS thing. This time I'm using the proper way to style 'modifiers' like --muted etc.. And I'm really hoping we can completely remove the 'legacy' styling - that's why I separated those to the bottom of the SCSS files.

@jiromaykin jiromaykin force-pushed the feature/2480-add-NLDS-paragraphs branch from 29aaf04 to 63f119f Compare May 28, 2024 18:37
@jiromaykin jiromaykin requested a review from Bartvaderkin May 28, 2024 18:43
@alextreme
Copy link
Member

Adding 1.17.2 tag but considering the amount of work left I'm not sure if that is applicable

@jiromaykin jiromaykin force-pushed the feature/2480-add-NLDS-paragraphs branch 2 times, most recently from e87ff02 to 1f02c18 Compare May 30, 2024 12:00
@jiromaykin jiromaykin force-pushed the feature/2480-add-NLDS-paragraphs branch 2 times, most recently from e528595 to 3906b21 Compare May 30, 2024 20:14
@jiromaykin jiromaykin requested a review from pi-sigma May 30, 2024 21:00
Copy link
Contributor

@pi-sigma pi-sigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebasing on develop will create a merge conflict due to pdc/tests/test_views.py. I would fix it myself, but rebasing also marked a bunch of other files as having changed, so we should wait until Monday. Will take this up with Jiro first thing in the morning.

@jiromaykin jiromaykin force-pushed the feature/2480-add-NLDS-paragraphs branch from 3906b21 to 674573f Compare June 2, 2024 19:58
@jiromaykin jiromaykin requested a review from pi-sigma June 3, 2024 07:26
@alextreme alextreme removed the request for review from Bartvaderkin June 3, 2024 12:15
@alextreme alextreme requested a review from swrichards June 3, 2024 12:16
@alextreme alextreme merged commit d67dca7 into develop Jun 4, 2024
16 checks passed
@alextreme alextreme deleted the feature/2480-add-NLDS-paragraphs branch June 4, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants